Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Re-implemented from scratch integration testing #1716

Merged
merged 2 commits into from
Mar 13, 2018

Conversation

mssola
Copy link
Collaborator

@mssola mssola commented Mar 7, 2018

Summary

This PR includes two commits:

  1. The first one simply nukes the previous infrastructure.
  2. Then I've implemented from scratch a reliable integration tests framework.

Fixes #1667

They were buggy and we need to take another approach.

Signed-off-by: Miquel Sabaté Solà <[email protected]>
@mssola mssola requested a review from vitoravelino March 7, 2018 15:47
@mssola
Copy link
Collaborator Author

mssola commented Mar 8, 2018

As you can see I'm fighting Travis 😅 Review once I'm done.

@mssola
Copy link
Collaborator Author

mssola commented Mar 9, 2018

@vitoravelino this is ready to be reviewed. Travis is passing, yay! 😁

require "portus/test"

# TODO: on a Travis PR, only test portus:development and registry:2.{5,6}
# TODO: otherwise, test with portus:2.3 and portus:head
Copy link
Collaborator Author

@mssola mssola Mar 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vitoravelino this has been left out on purpose. I will tackle this in a subsequent PR. The idea is that running the whole matrix takes around 40 minutes on Travis, which is simply unbearable for mere PRs. So, after merging this PR, I'll send another which:

  • On PR it will just run the development image. In total it would then take around 20 minutes.
  • If this is not a PR (e.g. a PR has just been merged and travis is performing a run against the fresh master branch), then we will test with 2.3 and head. This should take around 30 minutes.

If we see that overall we are taking too much time on Travis, then we could start thinking on parallelizing (e.g. running the integration tests on a Jenkins job instead of Travis).

# Ruby tests
__database restart
bundle exec rspec spec
__database stop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any specific reason to restart and then stop the database?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because unit tests require the DB to be up, and integration tests raise their own database, so we should stop the main DB so there are no clashes.

for f in $TESTS; do
tests+=("$ROOT_DIR/spec/integration/$f.bats")
done
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace tabs with spaces.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

test), and we are using a couple of helpers defined in
`spec/integration/helpers.bash`:

TODODOTODODODO
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover or incomplete?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

vitoravelino
vitoravelino previously approved these changes Mar 12, 2018
Copy link
Contributor

@vitoravelino vitoravelino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kudos! 👍

@vitoravelino
Copy link
Contributor

This PR also fixes #1714, right?

I've also fixed a couple of small nitpicks in the current testing setup.

Fixes SUSE#1667

Signed-off-by: Miquel Sabaté Solà <[email protected]>
@mssola mssola merged commit 976a729 into SUSE:master Mar 13, 2018
@mssola mssola deleted the integration-testing branch March 13, 2018 07:45
@mssola
Copy link
Collaborator Author

mssola commented Mar 13, 2018

This PR also fixes #1714, right?

In principle yes, but I still want to make sure that it works out of the box outside of the integration tests.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants